Allow empty object storage cluster#2028
Conversation
Share cluster resolution via resolveClusterRead so getQueryProcessingStage matches read when object_storage_cluster_fallback_if_empty is enabled, and skip local object_storage_cluster lookup when object_storage_remote_initiator and object_storage_remote_initiator_cluster are both set. Co-authored-by: Cursor <cursoragent@cursor.com>
Cover pure fallback on unknown cluster, aggregate planning, remote-initiator interaction, and integration scenarios with locally unknown object_storage_cluster. Co-authored-by: Cursor <cursoragent@cursor.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bc7aacf2e4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const bool defer_object_storage_cluster_resolution | ||
| = settings[Setting::object_storage_remote_initiator] | ||
| && !settings[Setting::object_storage_remote_initiator_cluster].value.empty(); |
There was a problem hiding this comment.
Preserve remote-only initiator reads
When object_storage_remote_initiator=1 and only object_storage_remote_initiator_cluster is set on the initiator, this condition now forces fallback_to_pure=false even though there is no local object_storage_cluster to defer. read then takes the clustered path and rewrites the remote query with make_cluster_function=true, producing s3Cluster('', ...) instead of the previous s3(...); on remote users whose profile defines object_storage_cluster, that bypasses the fallback table function and the read runs only on the remote initiator, regressing the existing test_object_storage_remote_initiator_without_cluster_function case that expects replica subqueries. Only defer local cluster resolution when cluster_name_from_settings is non-empty.
Useful? React with 👍 / 👎.
Cover stateless and integration cases where object_storage_cluster is missing locally and remote initiator falls back to non-cluster execution. Co-authored-by: Cursor <cursoragent@cursor.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1ad4ce9652
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| if (!defer_object_storage_cluster_resolution | ||
| && !result.fallback_to_pure | ||
| && settings[Setting::object_storage_cluster_fallback_if_empty]) |
There was a problem hiding this comment.
Restrict fallback to object_storage_cluster setting
Because this check is in the IStorageCluster base and only looks at the resolved cluster name, enabling object_storage_cluster_fallback_if_empty also changes explicit cluster table functions such as s3Cluster('typo', ...): getClusterName returns the function argument, tryGetCluster returns null, and the read falls back to pure_storage on the initiator instead of reporting the bad cluster. This makes a misspelled or temporarily empty explicit cluster silently run as a single-node read whenever the setting is enabled, so the fallback needs to be gated to names that actually came from the object_storage_cluster setting or otherwise kept out of explicit *Cluster calls.
Useful? React with 👍 / 👎.
…uster functions. Distinguish s3() with object_storage_cluster setting from s3Cluster() argument so fallback works for the former but explicit cluster names still fail when unknown. Co-authored-by: Cursor <cursoragent@cursor.com>
Distinguish alternative-syntax table functions from table reads so remote initiator keeps s3() for fallback and uses cluster path for Iceberg tables. Co-authored-by: Cursor <cursoragent@cursor.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 92a51421bd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| auto remote_initiator_cluster_name = settings[Setting::object_storage_remote_initiator_cluster].value; | ||
| if (!remote_initiator_cluster_name.empty()) | ||
| { |
There was a problem hiding this comment.
Default remote initiator to
object_storage_cluster
When object_storage_remote_initiator=1 is used with alternative syntax and only object_storage_cluster is set, this new branch leaves remote_initiator_cluster unset because object_storage_remote_initiator_cluster is empty. That was a supported path: the old code defaulted the remote initiator cluster to cluster_name_from_settings, but now getQueryProcessingStage/read either throw BAD_ARGUMENTS or, with object_storage_cluster_fallback_if_empty=1, run the read locally instead of on a remote initiator. Please preserve the previous default to the object-storage cluster when the explicit remote-initiator cluster setting is absent.
Useful? React with 👍 / 👎.
| if (!cluster_name_in_settings && !getOriginalClusterName().empty()) | ||
| return false; |
There was a problem hiding this comment.
Apply fallback to table-engine
object_storage_cluster
This disables object_storage_cluster_fallback_if_empty for persistent S3/Iceberg tables created with SETTINGS object_storage_cluster='...', because those tables store that setting in getOriginalClusterName while cluster_name_in_settings remains false. As a result, SELECT ... SETTINGS object_storage_cluster_fallback_if_empty=1 against such a table still resolves the missing/empty cluster normally and throws instead of falling back to the local storage, even though the new setting is described as applying when object_storage_cluster is set. Please distinguish table-engine settings from explicit *Cluster arguments before rejecting the fallback.
Useful? React with 👍 / 👎.
… cluster setting. Track explicit *Cluster function arguments separately from table engine and query object_storage_cluster settings so fallback works for persistent tables. Co-authored-by: Cursor <cursoragent@cursor.com>
|
@codex review |
|
Codex Review: Didn't find any major issues. You're on a roll. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
| DECLARE(String, object_storage_cluster, "", R"( | ||
| Cluster to make distributed requests to object storages with alternative syntax. | ||
| )", EXPERIMENTAL) \ | ||
| DECLARE(Bool, object_storage_cluster_fallback_if_empty, false, R"( |
There was a problem hiding this comment.
Maybe rename it to object_storage_cluster_fallback_to_local_if_empty
| return getOriginalClusterName(); | ||
| } | ||
|
|
||
| bool StorageObjectStorageCluster::useObjectStorageClusterFallbackIfEmpty(ContextPtr context) const |
There was a problem hiding this comment.
It seems like I don't understand this PR enough. From the description, I got the idea that if the remote cluster isn't available (i.e, no nodes alive), the setting would allow to execute it on this machine.
But the method useObjectStorageClusterFallbackIfEmpty, which indicates it is related to that setting, doesn't even check that setting.
There was a problem hiding this comment.
Agree, I'll change it on more clean code.
Split storage policy from the setting check and route all local-fallback decisions through one helper so the resolve/read path is easier to follow. Co-authored-by: Cursor <cursoragent@cursor.com>
… syntax. Pure-send to a remote initiator requires object_storage_remote_initiator_cluster; otherwise keep the clustered path that defaults the initiator cluster to object_storage_cluster. Co-authored-by: Cursor <cursoragent@cursor.com>
…ster. Under remote-initiator deferral, an empty local cluster name must fall back to pure send so ENGINE=S3/Iceberg without object_storage_cluster does not hit LOGICAL_ERROR. Co-authored-by: Cursor <cursoragent@cursor.com>
Drop the redundant pure-send gate, flatten fallback_to_pure, and reuse the already resolved remote-initiator cluster on the clustered path. Co-authored-by: Cursor <cursoragent@cursor.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9e1b15fbcd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| /// Allow a missing remote-initiator cluster only when we would fall back to a pure/local read anyway. | ||
| const bool allow_null = local_fallback && result.fallback_to_pure; | ||
| result.remote_initiator_cluster = getClusterImpl( | ||
| context, | ||
| remote_initiator_cluster_name, | ||
| /*max_hosts*/ 0, | ||
| allow_null); |
There was a problem hiding this comment.
Do not ignore a missing remote initiator cluster
When object_storage_remote_initiator_cluster is supplied but misspelled or its swarm is empty, this allow_null path makes getClusterImpl return nullptr; the read path then falls through to readFallBackToPure whenever object_storage_cluster_fallback_to_local_if_empty is enabled. That silently runs a query that explicitly requested a remote initiator on the local server instead of reporting the bad initiator cluster, while the new fallback is documented for an empty or unknown object_storage_cluster, not for a missing initiator cluster.
Useful? React with 👍 / 👎.
Local fallback applies only to object_storage_cluster; a bad remote-initiator cluster must still report CLUSTER_DOESNT_EXIST. Co-authored-by: Cursor <cursoragent@cursor.com>
Empty OSC with remote_initiator and no remote_initiator_cluster must keep BAD_ARGUMENTS even when fallback is enabled; extend tests for cases 1-3 and this regression. Co-authored-by: Cursor <cursoragent@cursor.com>
Pure-send ENGINE tables under remote-initiator deferral (preserving OSC in SETTINGS/context) so fallback matches s3() alternative syntax; extend TF and ENGINE tests for cases 1-3. Co-authored-by: Cursor <cursoragent@cursor.com>
Merge the two policy hooks into usesObjectStorageClusterSettingSyntax, share local-vs-remote fallback decisions between read and getQueryProcessingStage, and dedupe remote-initiator send. Co-authored-by: Cursor <cursoragent@cursor.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e0c3a5d716
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| result.fallback_to_pure = cluster_name_from_settings.empty() | ||
| || (defer_object_storage_cluster_resolution && usesObjectStorageClusterSettingSyntax()); |
There was a problem hiding this comment.
Preserve
*Cluster rewrite unless fallback is needed
With object_storage_remote_initiator=1, object_storage_remote_initiator_cluster set, and a valid non-empty object_storage_cluster, this condition forces the read into the pure remote path even when object_storage_cluster_fallback_to_local_if_empty is false. That bypasses the later make_cluster_function=true rewrite to s3Cluster/icebergCluster that this file still documents as needed for old ClickHouse versions, so during rolling upgrades the remote initiator can receive s3(... SETTINGS object_storage_cluster=...) instead of the compatible *Cluster call and fail to parse or distribute it. Please keep the *Cluster path unless the cluster is actually missing/empty and fallback is requested.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Thanks for the review. The observation is correct that with object_storage_remote_initiator=1 and a non-empty object_storage_remote_initiator_cluster, setting-syntax reads (s3() / ENGINE ... SETTINGS object_storage_cluster=...) always take the pure remote path and send plain s3()/iceberg() rather than rewriting to *Cluster.
That is intentional and should stay.
Why not restore *Cluster when fallback is off
Under remote-initiator deferral, the initiator must not assume it knows how to resolve object_storage_cluster:
- OSC may be unknown / unset on the local node and defined only on the remote initiator (or in the remote user’s profile).
- Or the reverse: OSC is set on the initiator query, but the remote swarm is empty/unknown and should apply
object_storage_cluster_fallback_to_local_if_empty.
Sending s3Cluster('name', ...) (or equivalent) bakes the cluster name into the table-function argument. Explicit *Cluster never applies object_storage_cluster_fallback_to_local_if_empty, and it also forces the initiator to pick a concrete cluster-argument form even when the name is only meaningful on the remote side.
What we send instead
SELECT ... FROM remote('host', s3(...))
SETTINGS object_storage_cluster = '...', object_storage_cluster_fallback_to_local_if_empty = ...- Keep the plain
s3()/iceberg()function. - Pass
object_storage_clusteras a query setting (not a function argument). - If local OSC is empty, omit it so the remote can supply its own.
That lets the remote initiator distribute when it knows the cluster, or fall back locally when the setting allows — without the local node needing a live/known swarm definition.
The make_cluster_function=true / “old ClickHouse versions” rewrite remains for the non-deferred path (e.g. object_storage_remote_initiator=1 with only object_storage_cluster, no object_storage_remote_initiator_cluster), where the initiator cluster is the object-storage cluster itself and a *Cluster rewrite is appropriate.
So we will not change the deferral path back to *Cluster based on this comment.
…TTINGS. object_storage_cluster may be unknown locally and defined on the remote (or the reverse); *Cluster would bake the name into the function argument and skip remote fallback. Co-authored-by: Cursor <cursoragent@cursor.com>
|
@arthurpassos Hey Arthur, if you want to read more result of vibe coding, it is here. New setting
In other cases behavior should not be changed. |
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
Allow empty object storage cluster
Documentation entry for user-facing changes
With 'object_storage_cluster' setting query to s3,iceberg and some other sources are executed as cluster request.
But with swarm cluster, when initiator is not a cluster member, may be situation when no one swarm node is alive at the moment. In this case query is failed with
CLUSTER_DOESNT_EXISTerror.New setting
object_storage_cluster_fallback_if_emptyallow to execute read query on local node in this case.Write query is not executed on cluster right now, so attempt to write is still failed in this case to avoid situation when query is success when swarm is empty and failed when has some nodes alive.
PR is a little bit complex because:
s3(...)- can fall back ifobject_storage_clusteris emptys3(...) SETTINGS object_storage_remote_initiator=1- failed on local node ifobject_storage_clusteris emptys3(...) SETTINGS object_storage_remote_initiator=1, object_storage_remote_initiator_cluster='...'- decision about falling back must be made on remote initiator, on local nodeobject_storage_clustercan be unknown.But behavior is not changed for
Clusterfunctions:s3Cluster(...)- can't fall backs3Cluster(...) SETTINGS object_storage_remote_initiator=1- must failed on remote initiator ifobject_storage_clusteris empty.CI/CD Options
Exclude tests:
Regression jobs to run: